Can one of the admins verify this patch?

ok to test

Thanks for finding this! I would like to make sure that there isn't a more elegant solution that can avoid all the code duplication.

Can you post the full Valgrind and sanitizer errors, and the compile + run commands you used to get them?

Maybe @pdmullen could help me understand when he gets a chance: I guess I don't understand how the SuperTimeStepTaskList class can even call the function pointers to the public member functions of TimeIntegratorTaskList, when there is no direct inheritance relationship:

  } else if (id == PHY_BVAL) {
    task_list_[ntasks].TaskFunc=
        static_cast<TaskStatus (TaskList::*)(MeshBlock*,int)>
        (&SuperTimeStepTaskList::PhysicalBoundary_STS);
    task_list_[ntasks].lb_time = true;
  } else if (id == DIFFUSE_HYD) {
    task_list_[ntasks].TaskFunc=
        static_cast<TaskStatus (TaskList::*)(MeshBlock*,int)>
        (&TimeIntegratorTaskList::DiffuseHydro);
    task_list_[ntasks].lb_time = true;
  } else if (id == DIFFUSE_FLD) {
    task_list_[ntasks].TaskFunc=
        static_cast<TaskStatus (TaskList::*)(MeshBlock*,int)>
        (&TimeIntegratorTaskList::DiffuseField);
    task_list_[ntasks].lb_time = true;

This is how all TaskFuncs are called:

ret = (this->*task_list_[i].TaskFunc)(pmb, stage);

So when pststlist->DoTaskListOneStage(pmesh, stage); is called in the main timestepping function, it makes sense that this->PhysicalBoundary_STS(pmb, stage) is defined, but this->DiffuseField(pmb, stage) should be undefined, no?

Would making a copy of stage_wghts[] in the SuperTimeStepTaskList class constructor fix this? Since it has a pointer to the defined TimeIntegratorTaskList object, this is simple (but we would need to change the visibility of stage_wghts[] from private to public or make a Friend class relationship):

SuperTimeStepTaskList::SuperTimeStepTaskList(
    ParameterInput *pin, Mesh *pm, TimeIntegratorTaskList *ptlist) :
    sts_max_dt_ratio(pin->GetOrAddReal("time", "sts_max_dt_ratio", -1.0)),
    ptlist_(ptlist) {

But then I worry that dereferencing the function pointers still wouldn't refer to this object. It seems that this pointer is basically unused in the STS task list class, except for making a copy of the nstages variable:

A more sure-fire way to fix this would be to override DoAllAvailableTasks() with an STS-specific version that can dereference this or ptlist_ depending on the task ID

Thank you for your investigation. Here is the setting and the result on Mac (Sonoma 14.7, M1 tip, Apple clang version 16.0.0 (clang-1600.0.26.3))
I configured the settings as follows:
python configure.py --prob resist -b -sts
and run by the following command:
path/to/bin/athena -i athinput.txt

Makefile.txt
log_clang.txt
athinput.txt

We have performed the test by valgrind on a different machine. I will provide the information about it later.

Regarding the check by Valgrind:
python configure.py --prob resist -b -sts
valgrind --leak-check=full --show-leak-kinds=all --track-origins=yes --log-file=memory_log.txt /path/to/bin/athena -i athinput.txt -d output >> log_run
memory_log.txt

Thinking about this a bit more...

The original TaskList and TimeIntegratorTaskList class and inheritance design pre-dates me; maybe @tomidakn remembers the details of the original choices. The use of static_cast to upcast method pointers from TimeIntegratorTaskList methods to TaskList methods is totally fine as written:

• https://stackoverflow.com/questions/60000/c-inheritance-and-member-function-pointers
• https://stackoverflow.com/questions/4295117/pointer-to-member-conversion
• https://stackoverflow.com/questions/4272909/is-it-safe-to-upcast-a-method-pointer-and-use-it-with-base-class-pointer?noredirect=1&lq=1
• https://stackoverflow.com/questions/59228652/why-is-it-allowed-to-static-cast-a-method-of-a-derived-class-to-a-method-of-the

The addition of STS in #176 was also safe by the C++ standard, since all the task class method pointers were to SuperTimeStepTaskList methods:

The first time this inheritance design was extended in an unsafe way was in #257, a major 386-commit refactor, in which I removed the STS task functions that were identical to the original task functions, and I claimed in the writeup to change:

SuperTimeStepTaskList is now derived from TimeIntegratorTaskList and calls many of its parent class's TaskFunc instead of duplicating their implementations.

But I never did that! No idea if that was intentional/forgot to update the description, or if it was an oversight. The closest change to this I can find was passing a TimeIntegratorTaskList *ptlist to the ctor in
b5682c8 which doesnt seem used at all!

• Should we just change the inheritance to class SuperTimeStepTaskList : public TimeIntegratorTaskList and initialize the stage_wghts[]? And remove that ptlist pointer from the class.

Despite the unsafe change, it worked fine in practice because all of the task functions that STS shared with the main time integrator class operated only on their explicit function parameters, the MeshBlock pointer and the current int stage (and also the int ntasks, nstages in the shared parent class TaskList). It still is certainly undefined behavior according to the standard, but someone with more C++ compiler knowledge would need to explain how the method function pointer resolution works in the object files.

The addition of orbital advection in #321 made it unsafe in practice because it extended struct IntegratorWeight with new coefficients and Boolean flags, and those flags were suddenly referenced in the "shared" task functions e.g.

But even that has "been fine" for almost 4 years, since those conditionals likely worked correctly as the uninitialized values referenced by STS were never true!

retest this please